Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: onServerPrefetch #3070

Merged
merged 7 commits into from
May 7, 2021
Merged

feat: onServerPrefetch #3070

merged 7 commits into from
May 7, 2021

Conversation

Akryum
Copy link
Member

@Akryum Akryum commented Jan 20, 2021

Add a Composition API lifecycle hook onServerPrefetch that merge the passed handler function to the serverPrefetch component option.
Related to vuejs/apollo#1102

@Akryum Akryum requested a review from yyx990803 January 20, 2021 10:31
@Akryum Akryum self-assigned this Jan 20, 2021
@github-actions
Copy link

github-actions bot commented Jan 20, 2021 •
edited
Loading

Size report

Path Size
vue.global.prod.js 41.81 KB (+0.13% 🔺)
runtime-dom.global.prod.js 27.66 KB (+0.16% 🔺)
size-check.global.prod.js 16.84 KB (+0.24% 🔺)

@LinusBorg
Copy link
Member

LinusBorg commented Feb 1, 2021 •
edited
Loading

Would we technically consider this a new feature, requiring a minor release?

We're putting together 3.0.6, if we include this, we'd have to go to 3.1 instead.

Just mentioning as we had other features, not coming right now, labelled for 3.1. so we would have to relabel them for 3.2

@Akryum Akryum requested a review from HcySunYang February 3, 2021 11:08
@Akryum
Copy link
Member Author

Akryum commented Feb 3, 2021

The GH action can't download the actions/checkout action for some reason 🤷‍♂️

@HcySunYang HcySunYang mentioned this pull request Feb 3, 2021
Closed
@HcySunYang
Copy link
Member

@Akryum How about this #2902, it supports hooks from mixin/extend.

@Akryum
Copy link
Member Author

Akryum commented Feb 3, 2021

@Akryum How about this #2902, it supports hooks from mixin/extend.

I think both can co exist

@HcySunYang
Copy link
Member

Oh, sorry, I took a closer look at that PR(#2902), the conclusion is that these two cannot coexist because they basically do the same thing. The #2902 also provides an implementation of onServerPrefetch, and the implementation mechanism is the same as other hooks. That mechanism provides the ability to pause tracking, see injectHook pauseTracking and it's necessary. Of course, that PR(#2902) also has some flaws, but I think we can review and try to use it?

@HcySunYang
Copy link
Member

@LinusBorg @posva , Can you help me take a look at these two PRs(#2902 and #3070)? thanks.

@antfu antfu mentioned this pull request Feb 4, 2021
Closed
@LinusBorg LinusBorg added the need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. label Feb 4, 2021
@Akryum
Copy link
Member Author

Akryum commented Feb 8, 2021

I've taken some parts of #2902 to make it work like a lifecycle hook. Do we merge this PR into @tabjy's PR to give him credits?

Akryum
Akryum commented Feb 8, 2021
View reviewed changes
packages/server-renderer/src/render.ts
.then(() =>
Promise.all(prefetches.map(prefetch => prefetch.call(instance.proxy)))
)
.catch(() => {})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: error display is already done by the wrapped lifecycle hook function.

@tabjy
Copy link

tabjy commented Feb 8, 2021

Sorry I've been a bit busy recently.

I've taken some parts of #2902 to make it work like a lifecycle hook. Do we merge this PR into @tabjy's PR to give him credits?

That sounds like a lot of work. Alternatively, maybe you can add me as a co-author when merging this request? In which case please use Co-authored-by: tabjy <8033899+tabjy@users.noreply.github.com>.

I'll close #2902 once this is merged.

@tabjy tabjy mentioned this pull request Feb 8, 2021
Closed
@oliverzy
Copy link

How is this feature related to "Suspense + Async Setup"?

@araratmartirossyan
Copy link

I still have this issue with vite. A patch that I found in the issue fixed it but only for dev. After build it's coming back. Any updates on how it can be fixed on build time?

@ejez ejez mentioned this pull request Mar 6, 2021
Closed
danielroe added a commit to danielroe/vue-sanity that referenced this pull request Mar 7, 2021
@danielroe
feat!: partial/experimental support for vue3/vite
2110860 
Most notably this is lacking ssr support with vue3 pending vuejs/core#3070

see #148 for latest progress & status
@danielroe danielroe mentioned this pull request Mar 7, 2021
Closed
@HcySunYang HcySunYang added the ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. label Mar 29, 2021
@yyx990803 yyx990803 force-pushed the akryum/on-server-prefetch branch from 7edfc56 to 7a93cd5 Compare May 7, 2021 15:55
@yyx990803 yyx990803 merged commit 349eb0f into master May 7, 2021
@yyx990803 yyx990803 deleted the akryum/on-server-prefetch branch May 7, 2021 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HcySunYang HcySunYang HcySunYang approved these changes

@yyx990803 yyx990803 Awaiting requested review from yyx990803

Assignees

@Akryum Akryum

Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. scope: ssr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Akryum @LinusBorg @HcySunYang @tabjy @oliverzy @araratmartirossyan @yyx990803